-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nanocoap_vfs: add nanocoap_vfs_get() #17937
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any plans for nput
(cvfs-put
, coap upload
?) as well?
sys/shell/commands/sc_nanocoap_vfs.c
Outdated
return -EINVAL; | ||
} | ||
|
||
if (_is_dir(url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is too much relying on the file system semantics -- or whether it's just a practical default, especially as the trailing slash gives no practical local file name anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what if someone tries to download coap://host/path/
? If they give an output file name, that might be fully OK (given that host doesn't necessarily have file system semantics). If they do not given an output file name, strrchr(url, '/')
later will find the trailing slash, set dst = "/default-data/"
, and vfs_open
will fail to open that as a file for writing.
So I don't see a good reason for that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean if someone wanted to actually store the resource behind coap://host/path/
instead of doing a directory listing?
I'm not sure, is this really a use case for the shell utility? Maybe I have a narrow view because I only used few CoAP servers, but I thought <path>/
would always give a resource listing, not an actual resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's only a directory listing if what is behind that is a file server. If it's a pubsub broker it could be a topic, or on an RD it could be part of lookup. CoAP by itself has no concepts of files and directories, and even when acting as a client that saves data to a file system, we shouldn't make assumptions on the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet that in every second CoAP implementation a trailing slash would not survive the encoding / parsing process. See https://datatracker.ietf.org/doc/html/rfc7252#section-6.4 for details.
My understanding is that a trailing / should result in an additional empty URI-Path option. But I have the feeling that many implementation will just "normalize" that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been some, but they have been fixed. Servers for HTTP are more lax in that area, but a) CoAP tries to avoid some mistakes HTTP made, and b) unlike in HTTP there is no simple way to redirect to the other form (and serving the same document on can easily cause incorrect outcomes, which is why even the lax HTTP servers redirect).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But would it make sense to store this on the file system?
What do you mean?
My proposal is to simply remove this one check. Then any URI could be ncget'ted. It may make sense to have a check for the (possibly implicit) output file name to not be empty (as the file system error is likely not helpful to the user). Just like wget http://riot-os.org/ -Owebsite.html
makes sense, something like ncget coap://temperature.riot-os.org/temperatures/average/vienna/ latest-local.temp
can happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well right now if you do a ncget
on a /
path you get a parsed directory listing to stdout for the user's convenience:
> ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/
/core/include/
/core/cond.c
/core/doc.txt
/core/mbox.c
/core/msg_bus.c
/core/mutex.c
/core/thread_flags.c
/core/Makefile
/core/lib/
/core/msg.c
/core/Kconfig
/core/sched.c
/core/thread.c
If you to a ncget
on a 'file' it will be downloaded instead:
ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/thread.c
Saved as /nvm0/thread.c
unless you specify stdout as the destination
ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/thread.c -
/*
* Copyright (C) 2013 Freie Universität Berlin
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/
/**
* @ingroup core_thread
* @{
*
* @file
* @brief Threading implementation
…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well you're interacting with a file server -- but not every server is, and it's part of the flexibility of web protocols that you can. I don't regularly interact with IPSO servers, but IIRC almost all of their resources have trailing slashes.
I don't even object to the default destination of paths with trailing slashes being -
(as is implied here), that's a very sensible thing to do. But if a user wanted to save it (eg. because some device makes its most frequently accessed resource available on the empty path), that should not be ruled out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah then this is a simple change! - cfdd360
Now that |
@chrysn do you want to take another look? Otherwise I'd do some testing and (assuming positive outcome) give an ACK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't do a full check (as maribu already mentioned the pending ACK), but didn't find any blockers; address comments as you deem adequate.
sys/shell/commands/sc_nanocoap_vfs.c
Outdated
return -EINVAL; | ||
} | ||
|
||
if (_is_dir(url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what if someone tries to download coap://host/path/
? If they give an output file name, that might be fully OK (given that host doesn't necessarily have file system semantics). If they do not given an output file name, strrchr(url, '/')
later will find the trailing slash, set dst = "/default-data/"
, and vfs_open
will fail to open that as a file for writing.
So I don't see a good reason for that check.
* @returns 0 on success | ||
* @returns <0 on error | ||
*/ | ||
int nanocoap_vfs_get(nanocoap_sock_t *sock, const char *path, const char *dst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever there is a pair like this (get vs. get_url), I wonder when to use which, if the use cases overlap or whether there are combinations that would be needed. (For example, can one use an established socket with a URL that has query components, or a Uri-Host component?)
Here it's probably motivated by the duality of nanocoap_sock_get_blockwise
and nanocoap_get_blockwise_url
, where I should have asked the question earlier.
I'd appreciate a note on when to use which, but maybe there are no good answers until #13827 is through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nanocoap_vfs_get_url()
is just there for convenience to avoid boilerplate in the common case.
The path component (and everything that goes with it) will be parsed the same way for both in nanocoap_sock_get_blockwise()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the path argument is really more path and query? If so, that should be documented (dejavu, I think we've had this somewhere else already.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I just call it query path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only reviewed partially, but since I often forget to complete a review and leave the already provided comments in limbo, I rather click submit now half-way-through.
Should I squash? |
Please squash and rebase (: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Code looks good and I trust the provided testing.
* @returns 0 on success | ||
* @returns <0 on error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @returns 0 on success | |
* @returns <0 on error | |
* @retval 0 success | |
* @retval <0 error |
Contribution description
This adds a convenience function
nanocoap_vfs_get()
to download a file from a CoAP server and store it on the local filesystem.In addition a shell command
ncget
is provided that makes use of the function.Testing procedure
Run the
tests/nanocoap_cli
example. If you are onnative
you want to enable thevfs_auto_format
module to automatically create a file system on the auto-generated virtual storage.Run
aiocoap-fileserver .
in e.g. the RIOT root directory.Issues/PRs references